Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add linode UUID as a metadata to each node #162

Merged
merged 6 commits into from
Jan 5, 2024
Merged

feat: add linode UUID as a metadata to each node #162

merged 6 commits into from
Jan 5, 2024

Conversation

shanduur
Copy link
Member

@shanduur shanduur commented Jan 4, 2024

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

A host UUID was added to the Linode Instance in a client change: linode/linodego#284

Apply this UUID from the CCM to expose the underlying host uniqueness, can improve scheduling and placement decisions for resources deployed on clusters with CCM installed.

tchinmai7
tchinmai7 reviewed Jan 4, 2024
View reviewed changes
cloud/linode/annotations.go Outdated Show resolved Hide resolved
cloud/linode/client.go
linodeClient.SetBaseURL(validatedURL.String())

version := ""
matches := regexp.MustCompile(`/v\d+`).FindAllString(parsedURL.Path, -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still required after the linodego update?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same logic as in the linode-cosi-driver and linode-blockstorage-csi-driver, added there just so we have similar code paths in all 3 components.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a dumb question, but why dont we make a linodeClient.ParseURL which does this logic and sets the base url and version for you. this would move all this logic to the linodego and we could reuse it right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgarber-akamai @zliang-akamai for visibility on my dumb questions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @luthermonson, I think you are right. Let us discuss it and get back to you next week.

Copy link

@lgarber-akamai lgarber-akamai Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late follow-up! I think this is a good idea and I've created a ticket internally to get this implemented ^

cloud/linode/node_controller.go Outdated Show resolved Hide resolved
srust
srust requested changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@srust srust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good - agree with uuid -> host-uuid change.

srust
srust reviewed Jan 4, 2024
View reviewed changes
cloud/linode/annotations.go Outdated Show resolved Hide resolved
@shanduur shanduur force-pushed the feat/metadata branch 2 times, most recently from f1fc47f to d8f8b67 Compare January 4, 2024 19:02
@shanduur shanduur requested a review from srust January 4, 2024 19:02
@shanduur-akamai
fix: prevent backslash escape
21b185a 
Signed-off-by: Mateusz Urbanek <[email protected]>
@shanduur shanduur merged commit 5103be6 into linode:main Jan 5, 2024
4 checks passed
@shanduur shanduur deleted the feat/metadata branch January 5, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgarber-akamai lgarber-akamai lgarber-akamai left review comments

@luthermonson luthermonson luthermonson left review comments

@zliang-akamai zliang-akamai zliang-akamai approved these changes

@tchinmai7 tchinmai7 tchinmai7 approved these changes

@srust srust srust approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@shanduur @luthermonson @srust @tchinmai7 @lgarber-akamai @zliang-akamai @shanduur-akamai